Skip to content

Adding ESP32 support and fix for ESP32 v 1.0.2 #95

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Adding ESP32 support and fix for ESP32 v 1.0.2 #95

wants to merge 2 commits into from

Conversation

kmpelectronics
Copy link

We have added ESP32 support in Ethernet 2.0.0 library. Especially for ESP32
v1.0.2.
You can delete our comments where they are necessary.

Add ESP32 support
cdd6fa3 
Adding ESP32 support in Ethernet 2.0.0 library.  Especially for ESP32
v1.0.2
per1234
per1234 reviewed Apr 23, 2019
View reviewed changes
src/Ethernet.h
public:
EthernetServer(uint16_t port) : _port(port) { }
EthernetClient available();
EthernetClient accept();
// 20190421 https://kmpelectronics.eu/ Plamen Kovandjiev - We added support for ESP32.
#ifdef ESP32
virtual void begin(uint16_t port = 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related: #88
It seems like it would be better for the ESP32 core developers to follow the standard Arduino core API, rather than forcing every library to be patched to deal with the breakage they caused.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, the ESP folks appear to have strayed from the Arduino API. Not good.

src/Ethernet.h Outdated
@@ -219,6 +219,11 @@ class EthernetClient : public Client {
uint8_t status();
virtual int connect(IPAddress ip, uint16_t port);
virtual int connect(const char *host, uint16_t port);
// 20190421 https://kmpelectronics.eu/ Plamen Kovandjiev - We added support for ESP32.
#ifdef ESP32
virtual int connect(IPAddress ip, uint16_t port, int timeout);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a mechanism for setting the timeout:
https://www.arduino.cc/en/Reference/EthernetClientSetConnectionTimeout

@JAndrassy
Copy link
Contributor

JAndrassy commented Apr 24, 2019 •
edited
Loading

my comment there espressif/arduino-esp32@9a9ff62#commitcomment-33292990

the PR should be closed, not be merged

@kmpelectronics
Copy link
Author

kmpelectronics commented Apr 25, 2019 •
edited
Loading

Dear @JAndrassy
Why this method
virtual int connect(IPAddress ip, uint16_t port, int timeout);
has been added in ESP32 release v 1.0.2?
Also what about do you say for
virtual void begin(uint16_t port = 0);
method?
It isn't implemented in Arduino Ethernet.h.
How do we fix the problem with it implementation?

@JAndrassy
Copy link
Contributor

JAndrassy commented Apr 25, 2019 •
edited
Loading

Dear @JAndrassy
Why this method
virtual int connect(IPAddress ip, uint16_t port, int timeout);
has been added in ESP32 release v 1.0.2?
Also what about do you say for
virtual void begin(uint16_t port = 0);
method?
It isn't implemented in Arduino Ethernet.h.
How do we fix the problem with it implementation?

the ESP32 package should be fixed. they should not change the Arduino API classes

This was referenced Aug 6, 2019
Closed
Closed
per1234
per1234 requested changes Sep 16, 2019
View reviewed changes
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The need to have special declarations of EthernetClient::connect() for ESP32 was removed by espressif/arduino-esp32#3191.

Unfortunately, the situation with EthernetServer::begin() remains.

I don't know what the story is with EthernetServer::init(). @kmpelectronics, please provide an explanation regarding the need for this.

I don't like the "mark my territory" comments. You will be given credit for your contribution in the commit history and you are welcome to add your name to the AUTHORS file in this PR if you like. I think it's a bad idea to clutter up the code with this stuff though. Imagine what a mess the source files would be if everyone who touched these files had done that over the years!

@kmpelectronics
Copy link
Author

kmpelectronics commented Sep 23, 2019 •
edited
Loading

The method EthernetServer::init() prevent of duplicating the same code in EthernetServer::begin(uint16_t port) and EthernetServer::begin() methods.

@JAndrassy
Copy link
Contributor

JAndrassy commented Sep 23, 2019 •
edited
Loading

The method EthernetServer::init() prevent of duplicating the same code in EthernetServer::begin(uint16_t port) and EthernetServer::begin() methods.

Ethernet.init(pin) sets the CS pin

@kmpelectronics
Copy link
Author

Where is the problem with EthernetServer::init() it inherits Server and doesn't have connection with Ethernet.init(pin)... Do you have any ideas about the name of the private method EthernetServer.init()?

@JAndrassy
Copy link
Contributor

Where is the problem with EthernetServer::init() it inherits Server and doesn't have connection with Ethernet.init(pin)... Do you have any ideas about the name of the private method EthernetServer.init()?

sorry. yes. I mixed it. And what should be server.init()? EthernetServer has a constructor. init() is usually for additional initialisation of objects instanced in a library.

@kmpelectronics
Copy link
Author

I have removed EthernetClient::connect() specific method for ESP32 and renamed from EthernetServer::init() to EthernetServer::initSocket()

@JAndrassy
Copy link
Contributor

@kmpelectronics, port is specified in constructor. The ESP32 core changed Arduino API Server class's virtual void begin(); and should change it back to be without parameter as all networking libraries expect it in derived Server classes. Make a PR there.

@kmpelectronics
Copy link
Author

OK, I will close this PR. But you should know this library does not work together with ESP32.

@per1234 per1234 added conclusion: declined Will not be worked on topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Sep 1, 2024
@per1234 per1234 self-assigned this Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PaulStoffregen PaulStoffregen PaulStoffregen left review comments

@per1234 per1234 Awaiting requested review from per1234

Assignees

@per1234 per1234

Labels
conclusion: declined Will not be worked on topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kmpelectronics @JAndrassy @PaulStoffregen @per1234 @kovandzhiev